-
-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not run ChangeReadOnlyPropertyWithDefaultValueToConstantRector
on properties with attributes
#1694
Conversation
Seems like I'm responsible for tests failure, investigating |
It rather looks like bug between Symfony and Paratest with color string: https://github.com/rectorphp/rector-src/runs/4850475081?check_suite_focus=true#step:5:88 |
Yup, I noticed that but master is green 🤔 I'm waiting for test suite to finish on my local machine. |
There are ~3 test failures that are hidden in the output by the paratest bug you referenced. |
That usually means new version (of paratest) was release since last PR was merged. |
You can lock the paratest to last lower version in this PR to see if it affects result on CI. |
Last release was in December :D never mind, I'll try to fix that one too. |
That can be allowed by another dependency in another package. With composer you never know what you get :) |
True true |
Hm, the last passing PR (#1669) had: Locking brianium/paratest (v6.4.1) This one has the same: Installing brianium/paratest (v6.4.1): Extracting archive |
I see... When I check out this PR and run: pu rules-tests/Privatization/Rector/Property/ChangeReadOnlyPropertyWithDefaultValueToConstantRector/ChangeReadOnlyPropertyWithDefaultValueToConstantRectorTest.php The test fails. The paratest has some bug to interpret these changes correctly, that we discovered by accident. |
Without your change, only the added fixture fails. |
Yup |
I think I know :) |
My condition is wrong |
It's the naming issue. In 2011, when the php-parser was born, nobody knew the native annotations will come to PHP and will be called attributes.So Nikita used "attributes" name for the metadata informations. Now we have native I'd prefer to name first to "metadata" and second to "attributes", but in 2025 we might have native "metadata " feature in PHP and we're in the same position :) |
Feel free to update this PR, I can merge it then. Thanks 👍 |
…n properties with attributes
Cool. Was not familiar with the API so thanks for pointer. |
Thanks for the improvement 👏 I got burned this ~5 times already 😆 |
I tried to fix the paratest as well paratestphp/paratest#650 |
@simPod That's amazing, thank you 👍 |
No description provided.